Skip to content

Conversation

@wrongu
Copy link

@wrongu wrongu commented Feb 22, 2024

Description of the bug: The partial_credit and leaderboard decorators are class-based and use a class-based decorator pattern with @functools.wraps around an inner decorator method, along with a local callback function that is passed to the tests (i.e. set_score and set_leaderboard_value). The bug is that set_score and set_leaderboard_value update attributes of the wrong function when they are stacked. This means that only the outermost decorator with a callback function works. Example of the bug:

@partial_credit(1)
@leaderboard('a')
def test_a(set_score=None, set_leaderboard_value=None):
    set_score(1)  # works
    set_leaderboard_value(1) # fails silently, updating the __leaderboard_value__ of test_a but not of the outermost wrapper.

or, with the decorators in the other order:

@leaderboard('a')
@partial_credit(1)
def test_a(set_score=None, set_leaderboard_value=None):
    set_score(1)  # fails silently
    set_leaderboard_value(1) # works

Existing workaround: One can in principle avoid these issues by having separate partial_credit and leaderboard tests. However, this is not future-proof for adding new decorators. In fact, I discovered this bug because I added my own custom decorator for timeout, and I found that @timeout followed by @partial_credit failed while the reverse order behaved properly.

Fix: This PR fixes the issue by essentially emulating the functools.wraps behavior after the function is called (using update_wrapper), which ensures that the new values set by set_score and set_leaderboard_value are copied to the outer wrappers and thus visible to the test runner. I opted to use a context manager for this because it makes it easy to drop in a 1-line fix inside the decorators.

@wrongu wrongu requested a review from a team as a code owner February 22, 2024 14:58
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant